Skip to content

[SecuritySolution] Finishing touches on the alert prevalence#128295

Merged
janmonschke merged 6 commits intoelastic:mainfrom
janmonschke:security/alert-prevalence-refinement
Mar 24, 2022
Merged

[SecuritySolution] Finishing touches on the alert prevalence#128295
janmonschke merged 6 commits intoelastic:mainfrom
janmonschke:security/alert-prevalence-refinement

Conversation

@janmonschke
Copy link
Copy Markdown
Contributor

@janmonschke janmonschke commented Mar 22, 2022

Summary

After talking to product and design we came up with a list of smaller changes to touch up the alert prevalence work:

  • (a40ab7f) Use sentence case instead of double-upper case (is that the proper naming for that?)
  • (3c38cc4) When the prevalence request fails or is not available, we're now rendering the default empty value placeholder
  • (f4e5264) Adding the source event id to the highlighted fields.

Screenshot 2022-03-22 at 17 31 17

Checklist

Delete any items that are not applicable to this PR.

@janmonschke janmonschke added release_note:skip Skip the PR/issue when compiling release notes Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. auto-backport Deprecated - use backport:version if exact versions are needed Team:Threat Hunting:Investigations Security Solution Threat Hunting Investigations Team v8.2.0 labels Mar 22, 2022
@janmonschke janmonschke requested a review from a team as a code owner March 22, 2022 17:17
janmonschke
janmonschke commented Mar 22, 2022
View reviewed changes
Comment thread x-pack/plugins/security_solution/public/common/components/event_details/summary_view.tsx
<EuiIconTip
type="iInCircle"
color="subdued"
title="Alert Prevalence"
Copy link
Copy Markdown
Contributor Author

@janmonschke janmonschke Mar 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@michaelolo24 I assume this one was forgotten in the initial PR. Let me know if we should keep the English copy here.

Copy link
Copy Markdown
Contributor

@michaelolo24 michaelolo24 Mar 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, thanks! I missed this translation.

@janmonschke
Copy link
Copy Markdown
Contributor Author

janmonschke commented Mar 22, 2022

@monina-n @paulewing Notice that in the screenshot above it says Source event id in the highlighted fields instead of kibana.original_event.id. This is how it would look like without the custom label:
Screenshot 2022-03-22 at 14 51 24

Let me know if I should change it back to the field id ✌️

@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented Mar 22, 2022

Pinging @elastic/security-solution (Team: SecuritySolution)

michaelolo24
michaelolo24 approved these changes Mar 23, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@michaelolo24 michaelolo24 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for making these changes. LGTM 🚀 !

@michaelolo24 michaelolo24 added backport:skip This PR does not require backporting and removed auto-backport Deprecated - use backport:version if exact versions are needed labels Mar 23, 2022
@YulNaumenko YulNaumenko self-requested a review March 23, 2022 17:16
YulNaumenko
YulNaumenko reviewed Mar 23, 2022
View reviewed changes
Comment thread ...k/plugins/security_solution/public/common/components/event_details/table/prevalence_cell.tsx Outdated
return <EuiLoadingSpinner />;
} else if (error) {
return null;
return <>{getEmptyValue()}</>;
Copy link
Copy Markdown
Contributor

@YulNaumenko YulNaumenko Mar 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Maybe better to use defaultToEmptyTag?
But in general it also uses a getEmptyTagValue and getEmptyStringTag which are really the same, that is more than weird how many duplications we have :)
x-pack/plugins/timelines/public/components/empty_value/index.tsx needs a clean up.

Copy link
Copy Markdown
Contributor Author

@janmonschke janmonschke Mar 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, works either way I guess 👍 (e2f321f)

@janmonschke
Copy link
Copy Markdown
Contributor Author

janmonschke commented Mar 24, 2022

@elasticmachine merge upstream

@kibana-ci
Copy link
Copy Markdown

kibana-ci commented Mar 24, 2022

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
securitySolution 4.8MB 4.8MB +151.0B

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@janmonschke janmonschke merged commit 46f5c03 into elastic:main Mar 24, 2022
@janmonschke janmonschke deleted the security/alert-prevalence-refinement branch March 24, 2022 16:33
@nastasha-solomon nastasha-solomon mentioned this pull request Apr 5, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@michaelolo24 michaelolo24 michaelolo24 approved these changes

@YulNaumenko YulNaumenko YulNaumenko approved these changes

Assignees

No one assigned

Labels

backport:skip This PR does not require backporting release_note:skip Skip the PR/issue when compiling release notes Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:Threat Hunting:Investigations Security Solution Threat Hunting Investigations Team v8.2.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@janmonschke @elasticmachine @kibana-ci @michaelolo24 @YulNaumenko @kibanamachine